-
Notifications
You must be signed in to change notification settings - Fork 446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add X-RateLimit-* response headers as an opt-in feature #77
Conversation
cc @stevenscg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a little refactoring for ease of testing, and for separation of logic. However, the more I think about this functionality the more I want to discuss before we commit to adding it.
- Should the reset data also be added to the RateLimit response on a per limit basis?
- In the "more than one limit" scenario we reduce the information returned to the min limit remaining. Is this the behavior we want?
resetHeader(limitingDescriptor, now), | ||
} | ||
} else if limitCount > 1 { | ||
// If there is more than one limit, then picking one of them for the "X-RateLimit-Limit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we pick min for remaining, shouldn't we pick the accompanying Limit to fill the limitHeader?
@junr03 I have a high level comment. Why are we adding headers here at all vs. just populating values in the proto response, and then letting the caller of this service do what they want with the data? |
@mattklein123 Yep that was my larger question re: The idea here was to leverage (https://github.com/envoyproxy/envoy/blob/4a5f85809cef13be47873483789a5a7bb64ded7c/api/envoy/service/ratelimit/v2/rls.proto#L94) to get the data via headers downstream from the Envoy making the call for free. |
@junr03 what I'm saying is that I think the change to rls.proto to add |
@mattklein123 yeah, I think the two of us are on the same page. Seeing it here made me rethink the use of those headers in general i.e it is unclear to me if we should have taken the original Envoy PR. However, I see two levels here:
wdyt? Any additional comments @stevenscg |
Yes it's fine to keep this feature, but for the things we are using it for here I don't think we should be using header, we should be sending the data explicitly and letting Envoy send headers if it wishes. |
Same page. I am going to close this, and proceed per |
Has there been any action on this within Envoy? I couldn't find anything by searching Envoy Issues and PRs and this seems like an extremely useful feature (requirement for my current project). Ideally I'd prefer to not fork the ratelimit project with this PR merged. Any other way of achieving this? |
@leosunmo no, I did not have the cycles to implement as per |
Thanks for that @junr03. Unfortunately the main Envoy project is a bit outside my comfort zone so probably won't be able to pick that up. I'll try to create an issue at some point soon. |
Taking over #56
Original PR message:
Around August 2018, Envoy added a new field,
headers
to theRateLimitResponse
proto message, which can be used to give Envoy custom headers to attach to its response:envoyproxy/envoy#4015
With this PR the ratelimit service will populate this response field with three headers:
X-RateLimit-Limit
CurrentLimit.RequestsPerUnit
value for theRateLimitResponse_DescriptorStatus
if there is one. If more than one descriptor from the request has an associated limit, then this field is NOT included in the response.X-RateLimit-Remaining
LimitRemaining
value of theRateLimitResponse_DescriptorStatus
s returned by the Redis cache.X-RateLimit-Reset
RateLimit.Unit
and current time. If there are multiple descriptors, the one with the fewest remaining requests and longest time to reset is used.The behavior of populating these headers is disabled by default, so this change is perfectly backwards-compatible. The behavior must be enabled explicitly using the
RESPONSE_HEADERS_ENABLED
environment variable.The legacy rate limit API also still functions when
RESPONSE_HEADERS_ENABLED
istrue
, but the headers are dropped from the response during the conversion step.